Hibernate 7 - Step 2#15568
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…branches Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…overage Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- PropertyConfigSpec: 14 new tests covering column(Closure) with firstColumnIsColumnCopy, getJoinTableColumnConfig, configureExisting with pre-existing columns, column-delegate getters (getEnumType, getSqlType, getIndexName, getLength, getPrecision), toString, and clone with typeParams - HibernateAssociationQuerySpec: add negation() test covering L88 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…tructor test Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…der branch Add test covering L77-78: when componentBinder is set on the binder and the property is HibernateEmbeddedCollectionProperty, bindCollectionSecondPass delegates to componentBinder.bindEmbeddedCollectionComponent() and sets the result as the collection element. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Fix 17 plain executeUpdate('...') calls across 7 specs in
grails-test-examples/gorm to use executeUpdate('...', [:]).
H7's HibernateGormStaticApi rejects plain CharSequence args
(requires either a GString with interpolated params or the
Map overload).
- Add getDomainClasses() override to BookHibernateSpec in app1.
H7's HibernateSpec uses HibernateDatastoreSpringInitializer
which requires explicit domain class declaration; H5 auto-detected
via classpath scanning.
- Remove grails-test-examples-app1 and grails-test-examples-gorm
from h7IncompatibleProjects list — both now run cleanly under
Hibernate 7 via dependency substitution.
Remaining excluded: datasources (ChainedTransactionManager),
views-functional-tests (HAL/JSON diffs), scaffolding-fields
(grails-fields rendering diffs).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…urn types, cross-property arithmetic Bug 2: HibernateQueryExecutor.singleResult() now catches both org.hibernate.NonUniqueResultException and jakarta.persistence.NonUniqueResultException (H7 throws the JPA variant; the original catch missed it) and returns the first result instead of propagating. Bug 4: HqlQueryContext.aggregateTargetClass() now returns precise types per function: count() → Long, avg() → Double, sum/min/max() → Number. Previously all aggregates were bound to Long, causing QueryTypeMismatchException in H7's strict SQM type checking. Bug 5: Cross-property arithmetic in where-DSL (e.g. pageCount > price * 10) was silently dropped — the RHS property reference was coerced to a literal. Fixed via: - PropertyReference: Groovy wrapper returned by propertyMissing for numeric properties; *, +, -, / operators produce a PropertyArithmetic value object - PropertyArithmetic: value type carrying (propertyName, Operator, operand) - HibernateDetachedCriteria: H7-only DetachedCriteria subclass that overrides propertyMissing to return PropertyReference for numeric properties, and newInstance() to preserve the subtype through cloning - HibernateGormStaticApi: overrides where/whereLazy/whereAny to use HibernateDetachedCriteria as the closure delegate - PredicateGenerator: resolveNumericExpression() detects PropertyArithmetic and builds cb.prod/sum/diff/quot(path, operand) instead of a literal H5 and MongoDB are unaffected — all new types are confined to grails-data-hibernate7. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ve on managed entities H7 enforces strict collection identity during flush. GORM's addTo* and save() flow had two failure modes: 1. When an entity is already managed in the current Hibernate session, calling session.merge() causes H7 to create a second PersistentCollection for the same role+key alongside the one already tracked in the session cache -> 'Found two representations of same collection'. Fix (HibernateGormInstanceApi.performMerge): check session.contains(target) before merging. If the entity is already managed, skip merge entirely; dirty-checking and cascade will handle children on flush. 2. When addTo* is called on a managed entity, GormEntity.addTo uses direct field access (reflector.getProperty) which bypasses H7's bytecode-enhanced interceptor, sees null, and creates a plain ArrayList on the field. H7's session cache already tracks a PersistentBag/Set for that role -> two representations on the next save. Fix (HibernateEntity.addTo): override addTo in the H7 trait; for managed entities (id != null), trigger the H7 interceptor via InvokerHelper.getProperty to obtain the live PersistentCollection before delegating to GormEntity.super.addTo. Fix (HibernateEntityTransformation): re-target the concrete addToXxx generated methods so their internal addTo call dispatches through HibernateEntity.addTo rather than being hard-wired to GormEntity.addTo. Fix (HibernateGormInstanceApi.reconcileCollections): detect stale PersistentCollections (session != current session) and replace them with plain collections before merge, covering any edge cases where the H7 interceptor path is not taken. Adds AddToManagedEntitySpec with 4 tests covering: - addTo on an already-persisted entity - multiple addTo on a fresh transient entity - modify child + save twice - removeFrom + save Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…and proper applicationClass - Replace executeQuery(plainString) and executeUpdate(plainString) calls with the (String, Map) overloads (empty map for parameterless queries). HibernateGormStaticApi intentionally rejects plain String in the no-arg overload to prevent HQL injection; parameterless static queries must use the Map overload. - Add applicationClass = Application to @Integration so the spec shares the same application context and transaction manager as the other specs in this module, preventing test-data bleed between specs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
# Conflicts: # NOTICE # dependencies.gradle # gradle/publish-root-config.gradle
…ibernate7 # Conflicts: # .idea/codeStyles/Project.xml # NOTICE # build-logic/plugins/build.gradle # dependencies.gradle # gradle/publish-root-config.gradle
Following the pattern used in Hibernate 5, I have moved the vendored Spring Framework ORM Hibernate 7 integration classes from the core module to a dedicated spring-orm module:
- Created Module: grails-data-hibernate7-spring-orm (located in grails-data-hibernate7/spring-orm).
- Moved Classes: All classes in org.grails.orm.hibernate.support.hibernate7 (including HibernateTransactionManager, HibernateTemplate, LocalSessionFactoryBean, etc.) are now in the new module.
- Updated Dependencies:
- Added the new module to settings.gradle.
- Updated grails-data-hibernate7-core, grails-plugin, boot-plugin, and dbmigration to depend on the new spring-orm module.
- Updated gradle/publish-root-config.gradle to ensure the new module is included in the publishing process.
- HibernateDetachedCriteria.isNumericPropertyType: box primitive types before the Number.isAssignableFrom check so that domains declaring numeric properties as primitives (int/long/double/float/short/byte) work correctly in where-DSL arithmetic expressions. Method is protected to allow subclass overrides. - ToManyEntityMultiTenantFilterBinder.bind: add null guard for getHibernateAssociatedEntity() return value to prevent NullPointerException on partially-resolved associations. - grails-data-hibernate7/README.md: add grails-data-hibernate7-spring-orm to the Module Structure table. - Tests: new HibernateDetachedCriteriaSpec covering boxed and all 6 primitive numeric types, non-numeric delegation, and unknown property. Added null-associated-entity test to ToManyEntityMultiTenantFilterBinderSpec. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
# Conflicts: # grails-data-mongodb/core/src/test/groovy/org/apache/grails/data/mongo/core/GrailsDataMongoTckManager.groovy # grails-data-mongodb/core/src/test/groovy/org/grails/datastore/gorm/mongo/BeforeUpdatePropertyPersistenceSpec.groovy # grails-datamapping-core-test/src/test/groovy/org/grails/datastore/gorm/CustomAutoTimestampSpec.groovy # grails-datamapping-core-test/src/test/groovy/org/grails/datastore/gorm/NestedCriteriaWithNamedQuerySpec.groovy # grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/GormStaticApi.groovy # grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/query/NamedCriteriaProxy.groovy # grails-datamapping-tck/src/main/groovy/org/apache/grails/data/testing/tck/tests/NamedQuerySpec.groovy # grails-test-suite-uber/src/test/groovy/grails/compiler/DomainClassWithInnerClassUsingStaticCompilationSpec.groovy
|
I believe I have explanations or resolutions to all of my critical issues. These 3 remain:
|
# Conflicts: # grails-test-examples/hibernate7/criteria-extension/src/test/groovy/example/NumberLikeExpressionSpec.groovy
Preserving Parameter Names
There was a problem hiding this comment.
Why are we changing files in grails-data-hibernate5-core in this PR?
There was a problem hiding this comment.
the grails-datamapping-core project was changed in some cases, and resulted in the breakage of tck tests that were fixed by these changes.
There was a problem hiding this comment.
It looks like the deprecations are separate - did you want me to revert those changes?
There was a problem hiding this comment.
Yes. Also, reverting the changes to HibernateMappingContext, PropertyConfig does not seem to break anything.
Reverting HibernateProxyHandler only breaks a new test added to the hibernate5-core project.
The test in the TCK that breaks by reverting AbstractHibernateCriterionAdapter seem to have been changed substantially.
There was a problem hiding this comment.
Why are there changes/additions to the tests in hibernate5-core?
There was a problem hiding this comment.
In general I tried to avoid making changes in core but it was not totally possible
There was a problem hiding this comment.
Appreciate that @borinquenkid. Which "core" are you referring to?
| } | ||
| }); | ||
|
|
||
| criterionAdaptors.put(Query.SizeNotEquals.class, new CriterionAdaptor<Query.SizeNotEquals>() { |
There was a problem hiding this comment.
The size spec was rewritten and without this code, hibernate 5 was broken.
There was a problem hiding this comment.
Why was it rewritten?
Why was it not separated with @Requires?
There was a problem hiding this comment.
I believe it was rewritten to ensure it's testing the same APIs that both of them implement. There was a gap in hibernate 5. I'm actually against removing this one as it's a legitmate bug in hibernate 5
There was a problem hiding this comment.
If it is a bug, should it be added as a separate issue/pr to 7.0.x and merged up?
There was a problem hiding this comment.
The issue is sizeNe is not implemented in detached criteria. We are moving to the detached criteria as the only API, so I believe we want this longer term. We can back port the query part, but the tck is meant to test the intended methods - it can't test stuff that was removed in hibernate 7. We're now picking the gorm API as the default.
There was a problem hiding this comment.
I believe it was rewritten to ensure it's testing the same APIs that both of them implement. There was a gap in hibernate 5. I'm actually against removing this one as it's a legitmate bug in hibernate 5
I don't think we should change the tests. If there are differences between H5 and H7, we should document them with @Requires.
If there is a bug in H5, we should add a ticket/pr targeted against the appropriate release branch.
| * | ||
| * @return The row count | ||
| */ | ||
| public Number countResults() { |
✅ All tests passed ✅🏷️ Commit: 4f837a4 Learn more about TestLens at testlens.app. |
|
We should not fix random issues in this PR (#14716). |
|
@matrei So we should release a milestone with a known bug with a limited blast radius? Composite FK is not a small corner case. |
No, that’s not what I meant, sorry if I came across as overly direct. I’m trying to be clear. What I’m saying is that this shouldn’t become the catch-all M2 PR. At least for me, it’s difficult to review changes when they aren’t focused and instead span several unrelated issues. |
|
@matrei Here is the funny part that might not be apparent with so many changes... this is mostly a refactor. That is why the bug is in both branches. The rewrite happened in the HibernateCriteriaBuilder because Hibernate removed the Criteria API. There was also almost no class hierarchy specific to Hibernate, which made the code harder to maintain and understand. I am keeping the scalability issue in another branch, which has its own blast radius, but it should be easier to evaluate. |
Reopening. This PR replaces #15530
Please note that I split off a prerequisite PR #15654
This allows us to better see what changed between hibernate 5 & 7 - otherwise the hibernate 7 code looks like it was just added instead of changed. Commit a47d8cb is the original commit prior to this split if we need to compare this branch to that for any reason (mistakes, etc).